-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update 2.0-dev to Godot 4.2 + mingw support #151
base: 2.0-dev
Are you sure you want to change the base?
Conversation
This may not actually work yet, but it compiles.
I've only tested building within the msys2 UCRT env for now. Forcing static linking for builtin_openvr=no isn't ideal but I'm not sure how to get the DLL path without assuming it's msys2 and not just somewhere that happens to have pkg-config. Building with an openvr submodule also works.
Rather than get this up to date in here, just require it to be installed separately.
Lots of broken code, but the extension loads.
I kind of guessed at these, I couldn't figure out where the old names were defined.
Viewport is abstract and we can't inherit from it in an extension. I'm not sure if this is the correct fix, but it was the only obvious path forward.
This isn't really optimal, because the header name was just made up by the msys2 project.
This works around godotengine/godot#64975 the same way as the reference XR and TiltFive extensions.
Messed up columns/rows when making this compile again.
There's no actual openvr connection in the editor, so stop trying to create the overlay there.
Use res:// paths to find actions within the project, and use the proper globalizing function depending on whether we're running in the editor.
Didn't run into this earlier because I was forcing it static for all mingw builds, now both compilers are controlled by a new build variable. The name of the variable was chosen to match godot-cpp. As of this commit, building with MSVC appears to be fully working. It does produce a lot of type mismatch warnings which need to be investigated, I'm not sure if mingw is actually producing a different result or if it's just quieter about it.
Document new build flags (and old), remove old context about updates within the Godot 3.x train and old OpenVR, update example GDScript. I also added a lot of notes about requirements for successfully building with mingw/msys2.
Building with mingw while MSVC is installed didn't actually work because SCons generates the incorrect arguments. The easiest fix was just to do these TODOs now.
Godot has a debug_crt flag to put this back, but it conflicts with use_static_cpp so just punting until someone needs it.
a8608d4
to
af07c5f
Compare
This is always an annoying part of working with GDExtension. Godot internally can be compiled with double float support. Generally speaking, if it works for the downloadable version of Godot, I'm happy :)
I'm happy for that change, I do have plans to change XR tools in a way that it could be submoduled, but it has some downsides to maintaining it so it's unlikely that will happen soon.
Honestly, don't know either, just copied from the base implementation myself.
I'm surprised they re-added it as there is nothing that will be using it.. That said, OpenXR has introduced Mac support so maybe they are following suit? |
Note that CI needs to be updated with newer versions so the build checks run through. |
I've updated CI, though I can't guarantee it will work on the first try :) Edit: realized I can test this out on my fork, so I'll work out any issues on a different branch there. |
- Update scons to 4.x - Bump github action versions to eliminate node 12 deprecation warning. v4 of the asset actions contains breaking changes, so I stopped at v3 for now. - Combine build steps to be platform-agnostic where possible. - Remove dead macOS config - Add TODOs for outstanding tech debt
71dc283
to
80e9e45
Compare
There we go: https://github.com/vilhalmer/godot_openvr/actions/runs/8810090117 Apparently the action versions I updated to also throw deprecation warnings, but at least they're not... as deprecated? I'll plan on a second pass to get them to the latest versions soon. |
What is blocking this from merge? |
I haven't been pushing on it because I'm taking my time with the followups to make sure I haven't made any terrible API decisions, but I think this one is ready. I think Bastiaan just has a lot of higher priority projects :) I'll take a look and see if there are any stray commits I should tack on here in the next day or two. |
Having browsed through the changes I don't see anything that stands out to me as wrong. If other can test this and verify it is working as expected, I'm more than happy to merge this in. |
Once we do I think we also need to spend some time rejigging the branches. The master branch should probably be renamed to 3.x, while the 2.0-dev branch becomes our master branch. |
If we can avoid the microsoft redistributable it means that game player don't need to install https://learn.microsoft.com/en-us/cpp/windows/latest-supported-vc-redist?view=msvc-170 |
How did it go? |
I didn't find anything additional that should go in here, it should be good for people to test. |
In Godot Engine main we tend rebase and squash all commits into one, but not sure about GodotVR/godot_openvr. I would like squashing. |
I've just left it for review purposes since Github offers the option at merge time, but if that's not enabled on this repo I'll definitely squash. Might keep the build work in a separate commit. |
I had to pull the branch onto my fork to be able to build since all the builds expired. https://github.com/V-Sekai/godot_openvr/tree/2.0-dev-overlays |
Test plan:
|
The github actions only provide |
I had some trouble building for Godot Engine 4.3 so I rewrote the scons script rushed. Did not upgrade the demo project. Did not restore linux. Did not restore mingw. https://github.com/V-Sekai/godot_openvr/tree/2.0-dev-overlays-4.3-stable Note: took pieces from https://github.com/godotengine/godot-cpp-template |
I'll take a look at your changes tomorrow and see if I can figure out what's wrong in my version. I admittedly have never compiled it with a full Visual Studio environment, because I was specifically avoiding setting one up. I had it building with the bare tools installed via winget, but I haven't tried it again since I got it working. I want to do a lot of scons cleanup, but i was trying to avoid doing any more than I already did here. Maybe it would be better to just drop my build changes here and introduce a new PR for that once I have it reconciled. |
Sorry for the delay, Friday ended up being a very different day than I had planned.
Can you share the errors and some info about your build environment? Is it Visual Studio? |
I won’t be able to check for the next week but my goal is mingw, msvc and linux gcc and linux clang. |
Ok, some errors had crept in with my local code that aren't present in this branch, but when I check out this branch I'm able to run scons without any arguments with a local openvr checkout and get a build with the MSVC toolchain (warnings trimmed out):
I'm using the standalone build tools from here.
Were you building with |
That sounds great. I will be traveling soon so I might not able to test.
Please do. |
I'm back, what do we need to do? |
With these changes, OpenVR scene applications work again in Godot 4.2. The demo is functional. Overlays do not yet work, I'll have a followup PR to fix them with some changes for how they're created and rendered as discussed in Discord.
While I was adjusting the build system to work with latest godot-cpp, I also added support for building using mingw. I can attempt to break that into a separate PR if it would be easier to review, though I did most of my testing with a mingw build because I am new to development on Windows in general and keeping my usual toolchain made progress a lot faster. There were no code changes to support this, just some new logic for scons.
I did test MSVC builds from within the Build Tools environment, but I have not tested inside the IDE. There were no significant changes to that part of the SConstruct, but testing from someone more familiar with this whole toolchain would be very welcome. The MSVC build spits out a lot of type conversion warnings which gcc does not, and I intend to continue investigating those before this is merged to make sure they're safe and/or clean them up.
I'm currently without an actual headset to do final testing after getting MSVC working again. I should have it back in a week or two, but I figured I might as well get some other eyeballs on this in the meantime.
Things I'm not sure about
Other notes
This mingw support is not the cross-compilation support from #99, this is only support for compiling for Windows within a mingw/msys2 environment on Windows. I'll work on getting cross-compilation up to date in a separate PR if everything looks good, as I'd like to be able to build my app for both platforms within a single pipeline.
I can also clean up the commit history a bit if that's desired, I was figuring things out as I went and thought it would be best to preserve my train of thought for review.
I left some TODOs in SConstruct for other things I'd like to fix up to make the build more robust, but didn't want to balloon the changes any further just yet.